[feature][bug] Add tcp transport event dispatcher unsched flag & fix RDMA event dispatcher unsched flag#3238
[feature][bug] Add tcp transport event dispatcher unsched flag & fix RDMA event dispatcher unsched flag#3238MalikHou wants to merge 4 commits intoapache:masterfrom
Conversation
…atcher unsched flag
[feature][bugfix] Add tcp transport event dispatcher unsched flag & fix RDMA event dispatcher
There was a problem hiding this comment.
Pull request overview
This PR unifies “unsched” scheduling behavior across TCP and RDMA transports by introducing a single dispatcher-layer flag/helper, and updates transport event processing to use that unified decision (also correcting RDMA’s urgent/background selection).
Changes:
- Added
event_dispatcher_edisp_unschedandEventDispatcherUnsched()as the single source of truth for “unsched” behavior (with legacy RDMA flag OR’ed in when enabled). - Updated
TcpTransport::ProcessEventandRdmaTransport::ProcessEventto pickbthread_start_urgentvsbthread_start_backgroundviaEventDispatcherUnsched(). - Updated RDMA docs to describe the unified flag and deprecation of
rdma_edisp_unsched.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/brpc/tcp_transport.cpp | Switches ProcessEvent scheduling decision to EventDispatcherUnsched(). |
| src/brpc/rdma_transport.cpp | Uses unified unsched helper and fixes urgent/background selection for RDMA. |
| src/brpc/rdma/rdma_endpoint.h | Removes the legacy rdma_edisp_unsched declaration from an installed header. |
| src/brpc/rdma/rdma_endpoint.cpp | Removes the legacy rdma_edisp_unsched definition (re-homed elsewhere). |
| src/brpc/event_dispatcher.h | Declares new unified flag and helper (but has an incorrect semantics comment). |
| src/brpc/event_dispatcher.cpp | Defines new unified flag + legacy flag (deprecated) and implements EventDispatcherUnsched(). |
| docs/en/rdma.md | Documents unified flag semantics and deprecation (contains an incorrect example). |
| docs/cn/rdma.md | Same as English RDMA doc update (contains an incorrect example). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -41,6 +51,15 @@ static bvar::LatencyRecorder* g_edisp_read_lantency = NULL; | |||
| static bvar::LatencyRecorder* g_edisp_write_lantency = NULL; | |||
| static pthread_once_t g_edisp_once = PTHREAD_ONCE_INIT; | |||
|
|
|||
| bool EventDispatcherUnsched() { | |||
| #if BRPC_WITH_RDMA | |||
| return FLAGS_event_dispatcher_edisp_unsched || | |||
| rdma::FLAGS_rdma_edisp_unsched; | |||
| #else | |||
| return FLAGS_event_dispatcher_edisp_unsched; | |||
| #endif | |||
| } | |||
There was a problem hiding this comment.
EventDispatcherUnsched() and the new unified flag introduce non-trivial scheduling behavior changes, but there are no unit tests in the repository exercising the new flag/legacy-flag interaction or the urgent/background selection. Please add/adjust tests (e.g. in test/brpc_event_dispatcher_unittest.cpp) to cover: unified flag toggling, legacy rdma flag behavior when BRPC_WITH_RDMA, and that Tcp/Rdma ProcessEvent choose the intended bthread_start_* path.
There was a problem hiding this comment.
@yanglimingcn the evaluation here isn't very meaningful; I think your review would suffice.
[note] Solving document content issues
Event Dispatcher Unsched Flag
What problem does this PR solve?
Problem Summary:
rdma_edisp_unsched), while TCP had no unified switch.bthread_start_urgentandbthread_start_backgroundlogic was reversed, causing behavior opposite to expected unsched semantics.What is changed and the side effects?
Changed:
event_dispatcher_edisp_unsched.EventDispatcherUnsched()helper inevent_dispatcher.{h,cpp}as single source of truth.TcpTransport::ProcessEventandRdmaTransport::ProcessEventto useEventDispatcherUnsched():false->bthread_start_urgenttrue->bthread_start_backgroundbthread_start_urgentandbthread_start_backgroundbranches were previously swapped.rdma_edisp_unschedand kept compatibility through dispatcher-layer unified check.test/brpc_event_dispatcher_unittest.cpp:event_dispatcher_unsched_by_unified_flagevent_dispatcher_unsched_by_legacy_rdma_flag(whenBRPC_WITH_RDMA)tcp_unsched_true_returns_before_onedge_finishtcp_unsched_false_blocks_caller_when_single_workerEffects:
event_dispatcher_edisp_unsched=truemay reduce immediate foreground switching of dispatcher path.rdma_edisp_unschedis deprecated.Check List: